fix(coordinator): 会话状态机修复 — Starting 边沿 + Esc 取消支持 Processing (closes #51 #52)#76
Conversation
closes #51, #52 ## #51 — Starting 阶段忽略热键边沿 握手 ASR 期间(phase=Starting)的"停止"边沿(toggle 第二次按 / hold 松开) 之前被直接 _ => {} 掉,会话锁死直到用户再按一次或重启 app。 修: - SessionState 加 pending_stop: bool - handle_pressed Toggle/Starting → pending_stop = true - handle_released Hold/Starting → pending_stop = true - begin_session 转 phase=Listening 时 mem::replace 取出 pending_stop, 若为 true 立即 await end_session ## #52 — Esc 取消在 Processing 阶段无效 用户按 Esc 时若 end_session 已进入 Processing(润色 / 插入进行中),cancel 信号被忽略,文本仍插入到光标位置 → 数据无意写入用户当前 app。 修: - SessionState 加 cancelled: bool - cancel_session 设 cancelled=true(不再仅 Idle 之外 = 转 Idle) Processing 阶段保持 phase=Processing 让 end_session 自己走完检查 + 收尾 - end_session 在 ASR 完成后、polish 之前 + polish 完成后、insert 之前 各检查一次 cancelled,命中即跳过 insert/history.append 新会话 begin_session 入口清两个 flag,避免遗留触发奇怪行为。
Reviewer's GuideRefines the session coordinator state machine to correctly handle stop edges during the ASR Starting phase and to make Esc-based cancellation effective during Processing by tracking pending stop and cancellation flags and adding checks at key transition points. Sequence diagram for stop edge during Starting phasesequenceDiagram
actor User
participant HotkeyHandler as HotkeyHandler
participant Coordinator as Coordinator
participant SessionState as SessionState
participant ASRRecorder as ASRRecorder
User->>HotkeyHandler: press_hotkey_to_start
HotkeyHandler->>Coordinator: handle_pressed
Coordinator->>SessionState: set phase=Starting
Coordinator->>SessionState: pending_stop=false, cancelled=false
Coordinator->>ASRRecorder: begin_session_handshake
User->>HotkeyHandler: second_press_or_release_during_Starting
HotkeyHandler->>Coordinator: handle_pressed_or_released
Coordinator->>SessionState: pending_stop=true
ASRRecorder-->>Coordinator: recorder_start_ok
Coordinator->>SessionState: phase=Listening
Coordinator->>SessionState: read_and_clear_pending_stop
alt pending_stop_was_true
Coordinator->>Coordinator: end_session
else pending_stop_was_false
Coordinator->>Coordinator: continue_listening
end
Sequence diagram for Esc cancel during Processing phasesequenceDiagram
actor User
participant UI as UIHandler
participant Coordinator as Coordinator
participant SessionState as SessionState
participant ASR as ASREngine
participant Inserter as TextInserter
User->>UI: press_Esc
UI->>Coordinator: cancel_session
Coordinator->>SessionState: read phase
alt phase_is_Processing
Coordinator->>SessionState: cancelled=true
Coordinator->>ASR: cancel
Coordinator->>SessionState: keep phase=Processing
else phase_is_not_Processing
Coordinator->>SessionState: cancelled=true
Coordinator->>ASR: cancel_if_active
Coordinator->>SessionState: phase=Idle
end
ASR-->>Coordinator: transcription_ready
Coordinator->>Coordinator: end_session
Coordinator->>SessionState: check cancelled_before_polish
alt cancelled_true_before_polish
Coordinator->>SessionState: phase=Idle
Coordinator-->>UI: emit_cancelled_capsule
Coordinator-->>Inserter: skip_insert
else not_cancelled_yet
Coordinator->>Coordinator: polish_or_passthrough
Coordinator->>SessionState: check cancelled_after_polish
alt cancelled_true_after_polish
Coordinator->>SessionState: phase=Idle
Coordinator-->>UI: emit_cancelled_capsule
Coordinator-->>Inserter: skip_insert
else not_cancelled_after_polish
Coordinator->>Inserter: insert_polished_text
Coordinator->>SessionState: phase=Idle
end
end
Class diagram for updated SessionState and phasesclassDiagram
class SessionPhase {
<<enum>>
Idle
Starting
Listening
Processing
}
class SessionState {
+SessionPhase phase
+Instant started_at
+bool pending_stop
+bool cancelled
}
SessionState --> SessionPhase
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The state machine flags (
pending_stop,cancelled) are now touched in several places with separateinner.state.lock()calls; consider centralizing these transitions into small helper methods onSessionState(e.g.queue_stop_edge_during_starting,consume_pending_stop,mark_cancelled) so that phase changes and flag updates always happen under a single, well-defined lock scope. - In
cancel_session,cancelledis set for any non-Idle phase even whenphase != Processing, but in those branches you immediately move toIdleandend_sessionwill never observe the flag; consider only settingcancelledwhenphase == Processing(or clearing it when you forceIdle) to avoid confusing, stale state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The state machine flags (`pending_stop`, `cancelled`) are now touched in several places with separate `inner.state.lock()` calls; consider centralizing these transitions into small helper methods on `SessionState` (e.g. `queue_stop_edge_during_starting`, `consume_pending_stop`, `mark_cancelled`) so that phase changes and flag updates always happen under a single, well-defined lock scope.
- In `cancel_session`, `cancelled` is set for any non-Idle phase even when `phase != Processing`, but in those branches you immediately move to `Idle` and `end_session` will never observe the flag; consider only setting `cancelled` when `phase == Processing` (or clearing it when you force `Idle`) to avoid confusing, stale state.
## Individual Comments
### Comment 1
<location path="openless-all/app/src-tauri/src/coordinator.rs" line_range="414-416" />
<code_context>
- inner.state.lock().phase = SessionPhase::Listening;
+ // 转 Listening 同时检查 Starting 期间是否积累了 pending_stop 边沿。
+ // hold 模式快速松开 / toggle 快速双击会到这里:握手刚完就要立即停。
+ let should_stop_immediately = {
+ let mut state = inner.state.lock();
+ state.phase = SessionPhase::Listening;
+ std::mem::replace(&mut state.pending_stop, false)
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid blindly overwriting `phase` to `Listening` in case `cancel_session` ran concurrently
Within the `Recorder::start` success path, `state.phase` is always set to `Listening` inside `should_stop_immediately`. If `cancel_session` runs after `start` returns but before this lock is acquired, it may already have set `phase` to `Idle` and stopped the recorder/ASR. Resetting it to `Listening` can leave the coordinator in an inconsistent state (e.g., `Listening` with no active recorder). Please gate this assignment on the current `phase` (e.g., only transition `Starting -> Listening`) or no-op when the session has been cancelled/ended.
</issue_to_address>
### Comment 2
<location path="openless-all/app/src-tauri/src/coordinator.rs" line_range="611-615" />
<code_context>
}
+ // Processing 阶段 cancel 不能直接干掉 in-flight polish task(已经 await 了),
+ // 但可以打 cancelled 标记,让 end_session 在插入前检查并丢弃结果。
+ inner.state.lock().cancelled = true;
+
if let Some(rec) = inner.recorder.lock().take() {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using a single `state` lock in `cancel_session` to avoid racey decisions based on a stale `phase` snapshot
Right now `cancel_session` reads `phase` under one `state` lock, then later re-locks `state` to set `cancelled = true` and maybe change `phase` based on the earlier snapshot. If `phase` can change between those two lock scopes, you may make decisions using stale state. Consider holding the lock once and doing the `phase` read plus `cancelled`/`phase` updates in a single critical section so they stay consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let should_stop_immediately = { | ||
| let mut state = inner.state.lock(); | ||
| state.phase = SessionPhase::Listening; |
There was a problem hiding this comment.
issue (bug_risk): Avoid blindly overwriting phase to Listening in case cancel_session ran concurrently
Within the Recorder::start success path, state.phase is always set to Listening inside should_stop_immediately. If cancel_session runs after start returns but before this lock is acquired, it may already have set phase to Idle and stopped the recorder/ASR. Resetting it to Listening can leave the coordinator in an inconsistent state (e.g., Listening with no active recorder). Please gate this assignment on the current phase (e.g., only transition Starting -> Listening) or no-op when the session has been cancelled/ended.
| inner.state.lock().cancelled = true; | ||
|
|
||
| if let Some(rec) = inner.recorder.lock().take() { | ||
| rec.stop(); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Consider using a single state lock in cancel_session to avoid racey decisions based on a stale phase snapshot
Right now cancel_session reads phase under one state lock, then later re-locks state to set cancelled = true and maybe change phase based on the earlier snapshot. If phase can change between those two lock scopes, you may make decisions using stale state. Consider holding the lock once and doing the phase read plus cancelled/phase updates in a single critical section so they stay consistent.
单一原则:会话状态机的边沿和取消语义
两个紧密相关的 bug,都在 `coordinator.rs` 的 phase 转换逻辑里:
#51 Starting 阶段忽略热键
握手 ASR 期间(`phase=Starting`)按下 stop(toggle 第二次按 / hold 松开),原本 `_ => {}` 掉,会话锁死。
修:`SessionState.pending_stop: bool`,握手完转 Listening 时 mem::replace 取出,若 true 立即 `end_session`。
#52 critical: Esc 取消在 Processing 阶段无效
用户按 Esc 时若 `end_session` 已进 Processing,cancel 被忽略,文本仍插入到光标。这是数据被无意写入用户 app 的 critical 问题。
修:`SessionState.cancelled: bool`,`cancel_session` 仅打标记不强转 Idle;`end_session` 在 polish 前 + insert 前各 check 一次。
Test plan
Summary by Sourcery
Fix session state machine to correctly handle stop edges during ASR startup and support cancellation throughout Processing without inserting text.
Bug Fixes: